Fet/rest relation endpoints (fixes #790) #798

mnelson4 · 2018-11-26T22:28:34Z

Problem this Pull Request solves

#790
Adds relation endpoints for simplifying adding and removing relations between individual model objects.
Also, fixes a bug/feature in the models that facilitated multiple entries in join tables having the same foreign keys.
Please see documentation added in this PR for more: https://github.com/eventespresso/event-espresso-core/blob/FET/rest-relation-endpoints/docs/C--REST-API/ee4-rest-api-editing-relations.md

How has this been tested

New features unit tested, so we should just verify there are no regressions by...

Verify the mobile apps still work fine
Verify the attendee mover works fine

Checklist

I have read the documentation relating to systems affected by this pull request, see https://github.com/eventespresso/event-espresso-core/tree/master/docs
User input is adequately validated and sanitized
all publicly displayed strings are internationalized (usually using esc_html__(), see https://codex.wordpress.org/I18n_for_WordPress_Developers)
My code is tested.
My code follows the Event Espresso code style.
My code has proper inline documentation.
My code accounts for when the site is in Maintenance Mode (MM2 especially disallows usage of models)

…sted

…e. Not sure why I thought that.

…rting or updating data

mnelson4 · 2018-11-28T05:03:09Z

First off, FYI I've written tentative documentation here: https://github.com/eventespresso/event-espresso-core/blob/FET/rest-relation-endpoints/docs/C--REST-API/ee4-rest-api-editing-relations.md

Several items I'd like Feedback on

would you expect POST /questions/1/question_groups/2 to add a new row to question_group_question every time a request is sent? (does it depend on the value of QGQ_order?)
In the case of question_group_question, we might prefer to avoid having duplicate rows (because you can't do that via the web UI). registration_payment I'm not sure if we want to allow multiple rows for how much each payment counts towards a registration's amount due...
I'm leaning towards allowing duplicate rows in join tables (because it's theoretically possible some models might want that, eg extra_join could); but I also realize that if someone really wants to do that, they can always directly create rows in the table, they don't need to use these convenience relation-editing endpoints.
and would you expect DELETE /questions/1/question_groups/2 to remove ALL entries in the join table or just one? I'm also slightly leaning towards deleting ALL join-table entries when you request to remove a relation (otherwise we'll need a way to indicate "request successfully deleted a join table row, but others exist" I think)
should I add entries to _links for these new routes? I don't really think so, as those don't specify which HTTP methods to send with the requests
Should URLs have singular entity names in them? eg is datetimes/1/event/2 better or worse than datetimes/1/events/2? Currently it's datetimes/1/event/2, as the routes are the same as the relation reading endpoints (eg GET datetimes/1/event gets the datetime's single event), it just also accepting another ID.

nerrad · 2018-11-28T13:52:00Z

would you expect POST /questions/1/question_groups/2 to add a new row to question_group_question every time a request is sent? (does it depend on the value of QGQ_order?)

Behaviour wise, what benefit is there to having multiple rows for that relation? Although it may be possible I think functionally wise, any code implementing this data would have the expectation there is only one record per question_group/question record right? So imo it should only allow one record per relation.

The other problem with allowing multiple records to get created is the side effect of how the join table data is represented in the _calculated_fields for a GET request on the relation endpoint (eg. questions/1/question_groups), currently it's just a singular representation of the join table data so that would be broken.

and would you expect DELETE /questions/1/question_groups/2 to remove ALL entries in the join table or just one?

Again, functionally speaking, what action performed by client code would have the expectation there'd be multiple relation rows for each question group -> question record? In other words, should we even be getting into the state of having multiple records for these relations in the table? So, on the off chance there are somehow, I'm fine with DELETE removing all entries in the table for that particular relation.

should I add entries to _links for these new routes? I don't really think so, as those don't specify which HTTP methods to send with the requests

You could have a self link for each entity returned in the response? That way client code knows that it can retrieve that entity schema etc via the self link.

Should URLs have singular entity names in them? eg is datetimes/1/event/2 better or worse than datetimes/1/events/2?

I think we'll have to be consistent with what the link in the schema is given for the relation (i.e. datetimes/1/event) because at least in my case that is what the code will use to build the relation endpoint. In hindsight I wish we had chosen to simply be consistent with ALL our endpoints regardless of whether they have multiple relations or not. If that's something you can account for here (i.e. accept both datetimes/1/events/2 and datetimes/1/event/2) and keep the existing schema reference the same for back compat that would rock. But if it could result in a significant performance hit don't worry about it.

mnelson4 · 2018-11-28T18:59:49Z

I want to be thrifty with adding more routes to the WP API index, as we've already got so many that we've had errors storing them in the WP options table (and generating them via the models on every REST API request is a significant time sink). So I'd rather stick with the existing routes, which are sometimes singular (eg datetimes/1/event/2).
The entities returned from a request like POST datetimes/1/event/2 DO have _links in them, including a self one. So I think we're good there.

kk, so to-do

when adding relations, if extra parameters don't match an existing row, update it
when removing relations, remove ALL
don't support extra query parameters when removing relations (REST API assumes only one per join entry)
update documentation

…sume only one row in join entries, so why would they want to provide the where query parameter when removing a relation

nerrad · 2018-12-27T14:58:50Z

I just want to highlight that this does block #790 from being complete as I need to wire things up there to work with the new endpoints and test appropriately. While I can merge this branch in, I'd be more comfortable with landing this in master first so I can avoid potential merge conflicts and this is baked a bit in master before releasing #790.

mnelson4 · 2018-12-27T18:57:52Z

I had heard we were asking Josh to do more of the triaging now, so passing to you Josh instead for assigning to someone for testing. (Please let me know if I should handle differently.)

joshfeck · 2018-12-27T22:14:52Z

tested & ready for merge, pending unit test fixes.

mnelson4 · 2018-12-27T22:32:49Z

It seems there was some change to Travis that's causing that (I think Brent was basically reporting the same issue here: https://eventespresso.slack.com/archives/C04J554E8/p1545942570001900).
So this is probably ok, but we should probably wait until this issue with Travis is resolved.
I think Darren has the best idea of how to fix the Travis issue. Darren, could you either merge this, or update it, when we're working ok with Travis again?

…relation endpoints (#798) * add routes for adding and removing relations * add validateModel method for reduring duplicated code in child classes * use validateModel to make code more DRY * theoretically, rest relation updating endpoints have been added. Untested * first test and a few fixes * relation adding and removing endpoints are unit tested * phpcbf fixes * import classes during unit tests * ModelDataTranslator::prepareFieldvalueForJson doesnt need the timezone. Not sure why I thought that. * import more classes * if they provide invalid parameters have an error, just like when inserting or updating data * deletion is successful if no row was found * update test expectations * update documentation to mention new relation editing endpoints * added item to REST API changelog * ouops all these tests should remove relations so be named like that * removing relations does not accept any parameters. These endpoints assume only one row in join entries, so why would they want to provide the where query parameter when removing a relation * assert that we remove ALL relations when removing relations not just one * consider join table entries as duplicates based only on foreign keys * update tests to make sure we update relations instead of adding duplicates in join tables when adding a relation but a slightly different one already exists * update tests to make sure we update relations instead of adding duplicates in join tables when adding a relation but a slightly different one already exists * phpcbf fix * update documentation * remove the unnecessary success property and document it * fix fatal error when running tests on phpunit4.8 * fixed some documentation typos as brent suggested "

#875)   ## Problem this Pull Request solves  Fixes an unreleased issue from #798 which caused primary attendee question groups to be disassociated with their event. See #873 This fixes it by, instead of only considering a join table row a duplicate based solely on its foreign keys, also take into account other columns (in the case of event-question-group, its `EQG_primary` value). This causes the primary question group relation to not get overwritten. **Note**: if you have an event on which you were reproducing #873, just switching to this branch and updating the event **won't** fix it. This just prevents the issue from occurring in the future for newly saved events. (IMO that's ok because this hasn't been released yet, so it's just us EE team who are inconvenienced- had it been released we'd want to retroactively fix those events) ## How has this been tested    * [ ] 1. Set up an event that requires the Personal Information Group for Additional Attendees 2. Select 2 or more tickets, and submit TS form 3. You'll be redirected to SPCO step 1 4. See missing attendee 1 form ## Checklist * [ ] I have read the documentation relating to systems affected by this pull request, see https://github.com/eventespresso/event-espresso-core/tree/master/docs * [ ] User input is adequately validated and sanitized * [ ] all publicly displayed strings are internationalized (usually using `esc_html__()`, see https://codex.wordpress.org/I18n_for_WordPress_Developers) * [ ] My code is tested. * [ ] My code follows the Event Espresso code style. * [ ] My code has proper inline documentation. * [ ] My code accounts for when the site is in Maintenance Mode (MM2 especially disallows usage of models)

…f only using FKs to identify a duplicate, use all the columns (#875)   ## Problem this Pull Request solves  Fixes an unreleased issue from #798 which caused primary attendee question groups to be disassociated with their event. See #873 This fixes it by, instead of only considering a join table row a duplicate based solely on its foreign keys, also take into account other columns (in the case of event-question-group, its `EQG_primary` value). This causes the primary question group relation to not get overwritten. **Note**: if you have an event on which you were reproducing #873, just switching to this branch and updating the event **won't** fix it. This just prevents the issue from occurring in the future for newly saved events. (IMO that's ok because this hasn't been released yet, so it's just us EE team who are inconvenienced- had it been released we'd want to retroactively fix those events) ## How has this been tested    * [ ] 1. Set up an event that requires the Personal Information Group for Additional Attendees 2. Select 2 or more tickets, and submit TS form 3. You'll be redirected to SPCO step 1 4. See missing attendee 1 form ## Checklist * [ ] I have read the documentation relating to systems affected by this pull request, see https://github.com/eventespresso/event-espresso-core/tree/master/docs * [ ] User input is adequately validated and sanitized * [ ] all publicly displayed strings are internationalized (usually using `esc_html__()`, see https://codex.wordpress.org/I18n_for_WordPress_Developers) * [ ] My code is tested. * [ ] My code follows the Event Espresso code style. * [ ] My code has proper inline documentation. * [ ] My code accounts for when the site is in Maintenance Mode (MM2 especially disallows usage of models) "

Mike Nelson added 4 commits November 22, 2018 11:30

add routes for adding and removing relations

7973911

add validateModel method for reduring duplicated code in child classes

0b5c0aa

use validateModel to make code more DRY

8269c41

theoretically, rest relation updating endpoints have been added. Unte…

393bb89

…sted

enhanced-triage-bot bot added the status:triage label Nov 26, 2018

Mike Nelson added 12 commits November 26, 2018 20:20

first test and a few fixes

2b8284a

relation adding and removing endpoints are unit tested

c6656fa

phpcbf fixes

a12d2bc

Merge branch 'master' into FET/rest-relation-endpoints

447b197

import classes during unit tests

9252e45

ModelDataTranslator::prepareFieldvalueForJson doesnt need the timezon…

8444d86

…e. Not sure why I thought that.

import more classes

48c5e9c

if they provide invalid parameters have an error, just like when inse…

624d839

…rting or updating data

deletion is successful if no row was found

9f848b8

update test expectations

2409adc

update documentation to mention new relation editing endpoints

64bde44

added item to REST API changelog

f5e3dbf

mnelson4 assigned mnelson4 and nerrad Nov 28, 2018

mnelson4 added status:feedback-needed type:feature-request 🙏 status:in-progress and removed status:triage labels Nov 28, 2018

nerrad removed their assignment Nov 28, 2018

nerrad removed the status:feedback-needed label Nov 28, 2018

Mike Nelson added 2 commits November 28, 2018 11:26

ouops all these tests should remove relations so be named like that

a43aa97

removing relations does not accept any parameters. These endpoints as…

4238659

…sume only one row in join entries, so why would they want to provide the where query parameter when removing a relation

tn3rb assigned mnelson4 and unassigned mnelson4 and tn3rb Dec 17, 2018

mnelson4 added status:needs-testing and removed status:review labels Dec 17, 2018

mnelson4 assigned sethshoultes Dec 17, 2018

mnelson4 added the status:triage label Dec 17, 2018

Merge branch 'master' into FET/rest-relation-endpoints

77afa00

Merge branch 'master' into FET/rest-relation-endpoints

00c747a

mnelson4 assigned joshfeck and unassigned sethshoultes Dec 27, 2018

joshfeck added status:merge and removed status:needs-testing status:triage labels Dec 27, 2018

mnelson4 assigned nerrad Dec 27, 2018

Merge branch 'master' into FET/rest-relation-endpoints

9adf8b6

nerrad merged commit 80062e4 into master Dec 30, 2018

nerrad deleted the FET/rest-relation-endpoints branch December 30, 2018 13:04

nerrad added this to the 4.9.76.p milestone Dec 30, 2018

nerrad changed the title ~~Fet/rest relation endpoints~~ Fet/rest relation endpoints (fixes #790) Dec 30, 2018

mnelson4 removed the status:merge label Dec 31, 2018

This was referenced Jan 3, 2019

instead of only using FKs to identify a duplicate, use all the columns #875

Merged

Fet/use indexes to decide habtm join table behaviour #877

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fet/rest relation endpoints (fixes #790) #798

Fet/rest relation endpoints (fixes #790) #798

mnelson4 commented Nov 26, 2018 •

edited by joshfeck

Loading

mnelson4 commented Nov 28, 2018 •

edited

Loading

nerrad commented Nov 28, 2018 •

edited

Loading

mnelson4 commented Nov 28, 2018 •

edited

Loading

nerrad commented Dec 27, 2018

mnelson4 commented Dec 27, 2018

joshfeck commented Dec 27, 2018

mnelson4 commented Dec 27, 2018

Fet/rest relation endpoints (fixes #790) #798

Fet/rest relation endpoints (fixes #790) #798

Conversation

mnelson4 commented Nov 26, 2018 • edited by joshfeck Loading

Problem this Pull Request solves

How has this been tested

Checklist

mnelson4 commented Nov 28, 2018 • edited Loading

Several items I'd like Feedback on

nerrad commented Nov 28, 2018 • edited Loading

mnelson4 commented Nov 28, 2018 • edited Loading

nerrad commented Dec 27, 2018

mnelson4 commented Dec 27, 2018

joshfeck commented Dec 27, 2018

mnelson4 commented Dec 27, 2018

mnelson4 commented Nov 26, 2018 •

edited by joshfeck

Loading

mnelson4 commented Nov 28, 2018 •

edited

Loading

nerrad commented Nov 28, 2018 •

edited

Loading

mnelson4 commented Nov 28, 2018 •

edited

Loading